Reviewed 3 of 9 files at r1.
Reviewable status: 3 of 9 files reviewed, 3 unresolved discussions (waiting on @aner-starkware)

a discussion (no related file):
I'd like to avoid (a) adding a new crate dependency just for pascal-casing and (b) avoid adding an extra field to the syscall processor.
instead, how about using the From<Felt> logic defined on the SyscallSelector?
see comments in your macro module on how I think this can work

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 43 at r1 (raw file):

                syscall_hint_processor.set_syscall_ptr(
                    get_ptr_from_var_name(Ids::SyscallPtr.into(), vm, ids_data, ap_tracking)?
                );

here, you can just fetch the integer at the current syscall pointer and verify it's the correct selector, no?
or is the pointer not pointing to the correct value at this point in time?
(see below, I'm assuming you have a $selector) macro ident available)

let syscall_ptr = get_ptr_from_var_name(Ids::SyscallPtr.into(), vm, ids_data, ap_tracking)?;
let selector = SyscallSelector::from(vm.get_integer(syscall_ptr)?)?;
if selector != SyscallSelector::$selector {
    return Err(..);
}
syscall_hint_processor.set_syscall_ptr(syscall_ptr);

Code quote:

                let syscall_hint_processor = &mut hint_processor.deprecated_syscall_hint_processor;
                syscall_hint_processor.set_syscall_name(
                    stringify!($name).to_string().to_case(Case::Pascal)
                );
                syscall_hint_processor.set_syscall_ptr(
                    get_ptr_from_var_name(Ids::SyscallPtr.into(), vm, ids_data, ap_tracking)?
                );

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 59 at r1 (raw file):

create_syscall_func!(
    call_contract,
    delegate_call,

... etc.
If paste has a pascal-case functionality you can use it, but if not, I wouldn't use an extra crate just to reduce the boilerplate here...

Suggestion:

create_syscall_func!(
    (call_contract, CallContract),
    (delegate_call, DelegateCall),

Reviewable status: 3 of 9 files reviewed, 3 unresolved discussions (waiting on @aner-starkware)

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 43 at r1 (raw file):

in the verification area, I'm not. see below, instead of just the function name I'm passing the selector name (function name in pascal case).
alternatively, you can pass CallContract instead of call_contract, and then you can get the function name by using paste:

paste::paste! {
    fn [< $selector:snake >]( .. ) { .. }
}

Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @aner-starkware, @ilyalesokhin-starkware, and @Yoni-Starkware)

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 43 at r1 (raw file):

OK, but in any case this solution checks it here, and not in verify_syscall_ptr - is this important?

this is a good question; can you check that it is equivalent? if the pointer doesn't change between this call and the verify call I think it's fine, but @Yoni-Starkware 's or @ilyalesokhin-starkware 's input may help

should I create a new Error type?

I am in favor, yes

Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @aner-starkware, @ilyalesokhin-starkware, and @Yoni-Starkware)

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 59 at r1 (raw file):

camel makes the first letter uppercase??

Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @aner-starkware, @ilyalesokhin-starkware, and @Yoni-Starkware)

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 52 at r2 (raw file):

                let syscall_selector = DeprecatedSyscallSelector::try_from(
                    felt_from_ptr(vm, &mut syscall_ptr)?
                ).unwrap();

1. no unwrap in business logic - if this cannot fail, use expect, with a string explaining why it cannot fail. if it can fail, return an error
2. does felt_from_ptr dereference the pointer? or just converts the address to felt?

Code quote:

                let syscall_selector = DeprecatedSyscallSelector::try_from(
                    felt_from_ptr(vm, &mut syscall_ptr)?
                ).unwrap();

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 53 at r2 (raw file):

                    felt_from_ptr(vm, &mut syscall_ptr)?
                ).unwrap();
                // TODO(Aner): should return an error instead of panic.

no reason to even merge it like this temporarily: add an error variant in the same PR, no problem (better, because it's in context)

Code quote:

// TODO(Aner): should return an error instead of panic.

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 59 at r2 (raw file):

                syscall_hint_processor.set_syscall_ptr(
                    syscall_ptr,
                );

format

Suggestion:

                paste!{
                    assert_eq!(syscall_selector, DeprecatedSyscallSelector::[<$name:camel>]);
                }
                syscall_hint_processor.set_syscall_ptr(syscall_ptr);

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @aner-starkware, @ilyalesokhin-starkware, and @Yoni-Starkware)

crates/blockifier/src/execution/deprecated_syscalls/hint_processor.rs line 110 at r3 (raw file):

    },
    #[error("Bad syscall_ptr; expected: {expected_ptr:?}, got: {actual_ptr:?}.")]
    BadSyscallPointer { expected_ptr: Relocatable, actual_ptr: Relocatable },

Suggestion:

    #[error("Bad syscall_ptr; expected: {expected_ptr:?}, got: {actual_ptr:?}.")]
    BadSyscallPointer { expected_ptr: Relocatable, actual_ptr: Relocatable },
    #[error("Bad syscall selector; expected: {expected_selector:?}, got: {actual_selector:?}.")]
    BadSyscallSelector {
        expected_selector: DeprecatedSyscallSelector,
        actual_selector: DeprecatedSyscallSelector,
    },

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 55 at r3 (raw file):

                    felt_from_ptr(vm, &mut syscall_ptr)?
                ).unwrap();
                // TODO(Aner): should return an error instead of panic.

delete

Code quote:

// TODO(Aner): should return an error instead of panic.

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 63 at r3 (raw file):

                        }));
                    }
                }

non blocking, but consider doing this: that way you only need the hacky pasteing once, and less indentation

Suggestion:

                let expected_selector = paste! { DeprecatedSyscallSelector::[<$name:camel>] };
                if syscall_selector != expected_selector {
                    return Err(DeprecatedSyscallExecution(DeprecatedSyscallExecutionError::BadSyscallSelector {
                        expected_selector,
                        actual_selector: syscall_selector,
                    }));
                }
        

Reviewable status: 8 of 10 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware, @ilyalesokhin-starkware, and @Yoni-Starkware)

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 43 at r1 (raw file):

1. The pointer doesn't change, and
2. The pointer is also saved to self (and later compared with the same pointer 🙄 )

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 52 at r2 (raw file):

Good catch! Thanks 🙏

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 53 at r2 (raw file):

Yeah, it was a temporary solution while I waited for your answer regarding the error.

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 59 at r2 (raw file):

Done.

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 55 at r3 (raw file):

Done.

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 63 at r3 (raw file):

Suggestion taken.

crates/blockifier/src/execution/deprecated_syscalls/hint_processor.rs line 110 at r3 (raw file):

    },
    #[error("Bad syscall_ptr; expected: {expected_ptr:?}, got: {actual_ptr:?}.")]
    BadSyscallPointer { expected_ptr: Relocatable, actual_ptr: Relocatable },

Done.

Reviewable status: 8 of 10 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware, @ilyalesokhin-starkware, and @Yoni-Starkware)

a discussion (no related file):

Done.

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aner-starkware)

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 59 at r2 (raw file):

the set_syscall_ptr line is still 3 lines instead of 1

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 67 at r4 (raw file):

                        );
                    }
                }

you don't need the paste here anymore

Code quote:

                paste!{
                    if syscall_selector != expected_selector {
                        return Err(
                            DeprecatedSyscallExecution(
                                DeprecatedSyscallExecutionError::BadSyscallSelector {
                                    expected_selector,
                                    actual_selector: syscall_selector,
                                }
                            )
                        );
                    }
                }

Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 59 at r2 (raw file):

Done.

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 67 at r4 (raw file):

Done.

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aner-starkware)

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 9 at r5 (raw file):

use paste::paste;

use crate::hints::error::OsHintError::DeprecatedSyscallExecution;

to not use enum variants please

Suggestion:

use crate::hints::error::OsHintError;

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 30 at r5 (raw file):

///     let syscall_hint_processor = &mut hint_processor.deprecated_syscall_hint_processor;
///     Ok(execute_next_deprecated_syscall(syscall_hint_processor, vm, ids_data, ap_tracking)?)
/// }

this docstring is no longer true

Code quote:

/// create_syscall_func!(get_block_number);
///
/// Expands to:
///
/// pub(crate) fn get_block_number<S: StateReader>(
/// HintArgs { hint_processor, vm, ids_data, ap_tracking, .. }: HintArgs<'_, '_, S>,
/// ) -> OsHintResult
/// {
///     let syscall_hint_processor = &mut hint_processor.deprecated_syscall_hint_processor;
///     Ok(execute_next_deprecated_syscall(syscall_hint_processor, vm, ids_data, ap_tracking)?)
/// }

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 64 at r5 (raw file):

                            }
                        )
                    );

this should work, you have a #[from] implemented

Suggestion:

                    return Err(
                        DeprecatedSyscallExecutionError::BadSyscallSelector {
                            expected_selector,
                            actual_selector: syscall_selector,
                        }
                        .into()
                    );

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 9 at r5 (raw file):

Done.

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 30 at r5 (raw file):

Done.

crates/starknet_os/src/hints/hint_implementation/syscalls.rs line 64 at r5 (raw file):

Done.

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)